-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor prior construction and make clearer the logic #566
Conversation
Try this Pull Request!Open Julia and type: import Pkg
Pkg.activate(temp=true)
Pkg.add(url="https://github.com/CDCgov/Rt-without-renewal", rev="new-way-to-specify-priors", subdir="EpiAware")
using EpiAware |
The idea of this approach is that the This passes to a function which then constructs the latent process in one of two ways:
|
The autocorrelation is expected to be 0.1. | ||
""" | ||
function _make_target_std_and_autocorr(igp; stationary::Bool) | ||
if igp == Renewal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This much nicer but my training for you makes me want it to dispatch down based on igp vs ifelse it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha thing here is that igp
is the type itself rather and an instance of a type so got to remember how to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it was indeed nicer!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## specific-priors #566 +/- ##
================================================
Coverage 90.96% 90.96%
================================================
Files 60 60
Lines 863 863
================================================
Hits 785 785
Misses 78 78 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is much nicer. I think it could get even nicer but happy with it as it is
Some of those priors look too confident at no outbreak |
As ever, Julia rewards thinking about dispatch. I think this version is getting there. Ready for re-review and sci review |
Nice: Prior predictive review:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation wise I think this is nice enough now (though I think there are a few alternative patterns that better match our more general approach and worth thinking about why/if they aren't easy to do).
So just the science to discuss now
Agree with all this |
Bumped the variance for models mentioned by @seabbs above. Think it looks ok... Obviously same |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree this looks good now
This PR is (hopefully) an improvement to #565 aimed at addressing @seabbs concerns about the implementation. Concerns I share because I think the opaque implementation got in the way of me implementing the priors I wanted (and @seabbs reviewing them from a scientific point of view).